-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Python Add the Amazon Neptune Python follow #7473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you've made the high-level fixes and it matches the specification, I can do a full review.
python/example_code/neptune/tests/test_neptune_scenario_integration.py
Outdated
Show resolved
Hide resolved
@pytest.fixture(scope="module") | ||
def neptune_client(): | ||
"""Create a real Neptune boto3 client for integration testing.""" | ||
client = boto3.client("neptune", region_name="us-east-1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the current testing patterns in the library until we decide as a team to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modelled tests after the Keyspace. Confirmed we follow pattern too
The Hello and Scenario examples run as excepted. The tests have been refactored to follow other Python tests such as Keyspace. The Spec was updated to include SME contributed code examples. |
print(f"Deleting DB Cluster: {cluster_id}") | ||
neptune_client.delete_db_cluster(**request) | ||
except ClientError as e: | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the exception handling for the ResourceNotFoundException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
try: | ||
db_cluster_id = create_db_cluster(neptune_client, cluster_name) | ||
except ClientError as ce: | ||
code = ce.response["Error"]["Code"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling should be in the function, not in the scenario, so that it is visible within the code snippet. Same for all handling, it should follow the pattern of being in the function with the client call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all Java sceanrio developed - the service method throws back the exception to the calling code where its handles and displayed. I assumed it was a better way for Python. Checked with Bedrock that relied:
" This is a good practice to handle the exceptions in the calling code, instead of displaying the exception in the service method. Here's why:
By handling the exceptions in the calling code, you're keeping the responsibility of the service method focused on its primary task, which is to delete the DB instance. The calling code is then responsible for handling the various exceptions that may arise, making the code more modular and easier to maintain."
Main entry point: create NeptuneGraph client and call graph creation. | ||
""" | ||
# Hypothetical client - boto3 currently doesn't have NeptuneGraph client, so replace with actual client if available | ||
neptune_graph_client = boto3.client("neptune") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hypothetical client? This seems incorrect, either there is a client for this code or there isn't. The code below seems to correctly create a neptune-graph
client, so why is this one hypothetical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
try: | ||
print("Creating Neptune graph...") | ||
|
||
# Hypothetical method for create_graph, adjust accordingly if you use HTTP API or SDK extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the method hypothetical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
try: | ||
response = client.execute_query( | ||
GraphIdentifier=graph_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capitalization in the parameters of all of these methods does not match the documentation and it also does not match the code provided by the SME:
`resp =client.execute_query(
graphIdentifier = graph_id,
queryString="MATCH (n {code: 'ANC'}) RETURN n",
language='OPEN_CYPHER'
)
print(resp['payload'].read().decode('UTF-8')`
Please recheck that the parameters match exactly on all these graph and data examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
# Import your scenario file; ensure Python can locate it. | ||
# If your file is named `neptune_scenario.py`, this import will work: | ||
import NeptuneScenario |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to run any of the tests that use the scenario file, I get an error: ModuleNotFoundError: No module named 'NeptuneScenario'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing all tests pass
================================================ 22 passed in 30.77s ====
# --- Success case --- | ||
mock_neptune = MagicMock() | ||
paginator_mock = MagicMock() | ||
mock_neptune.get_paginator.return_value = paginator_mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to use the established stubber pattern for the client responses, see the files in the test_tools
directory, and add a file such as neptune_stubber.py
for each client to stub the responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This did nto work from that folder. Only way it worked was to get it into local folder. No idea why.
|
||
As there is limited room in aboce table, the metadata key for `executeOpenCypherExplainQuery`is neptune_ExecuteOpenCypherExplainQuery. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can go back into the table, it seems to look fine there and will be harder to find here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
graph_name=graph_name | ||
) | ||
|
||
created_graph_name = response.get("GraphName") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is GraphName even in the response? I don't see it: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/neptune-graph/client/create_graph.html
And the other properties appear to be incorrectly named or capitalized as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - sure make it harder to verify without running code in IDE
print(resp['payload'].read().decode('UTF-8')) | ||
|
||
except client.exceptions.InternalServerException as e: | ||
print(f"InternalServerException: {e.response['Error']['Message']}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks for exceptions that don't exist for that client. For example, LimitExceededException is not in this list: https://botocore.amazonaws.com/v1/documentation/api/latest/reference/services/neptune-graph.html#client-exceptions
def execute_create_graph(client, graph_name): | ||
try: | ||
print("Creating Neptune graph...") | ||
response = client.create_graph(graph_name=graph_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code has an incorrect parameter and is missing a required parameter also.
This pull request adds the Amazon Neptune Python follow
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.